Open
Conversation
WalkthroughAdds a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove.go`:
- Around line 142-159: parseRemoveArgs duplicates the same "--" prefix
validation/stripping logic used by parseVolumeArgs; extract a small helper like
ensurePrefixed(arg string, prefix string) (or stripLeadingDashes(arg string))
that verifies strings.HasPrefix(arg, "--"), returns the remainder (trimmed) or
an error, and use it from both parseRemoveArgs and parseVolumeArgs to centralize
the validation and trimming logic; keep the existing empty-name checks (e.g.,
name == "") in each caller as-needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cmd/config/volumes/cmd.gocmd/config/volumes/remove.gocmd/config/volumes/remove_test.gocmd/config/volumes/set.gocmd/config/volumes/set_test.go
Comment on lines
+142
to
+159
| // parseRemoveArgs parses volume name arguments in the format --name. | ||
| func parseRemoveArgs(args []string) ([]string, error) { | ||
| names := make([]string, 0, len(args)) | ||
|
|
||
| for _, arg := range args { | ||
| if !strings.HasPrefix(arg, "--") { | ||
| return nil, fmt.Errorf("invalid volume name '%s': must start with --", arg) | ||
| } | ||
|
|
||
| name := strings.TrimSpace(strings.TrimPrefix(arg, "--")) | ||
| if name == "" { | ||
| return nil, fmt.Errorf("volume name cannot be empty in '%s'", arg) | ||
| } | ||
|
|
||
| names = append(names, name) | ||
| } | ||
|
|
||
| return names, nil |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared prefix-stripping logic.
Both parseRemoveArgs and parseVolumeArgs (in set.go) share similar -- prefix validation and stripping logic. While the formats differ (--name vs --name=path), a small helper for prefix validation could reduce duplication.
This is a minor observation; the current implementation is clear and correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/volumes/remove.go` around lines 142 - 159, parseRemoveArgs
duplicates the same "--" prefix validation/stripping logic used by
parseVolumeArgs; extract a small helper like ensurePrefixed(arg string, prefix
string) (or stripLeadingDashes(arg string)) that verifies strings.HasPrefix(arg,
"--"), returns the remainder (trimmed) or an error, and use it from both
parseRemoveArgs and parseVolumeArgs to centralize the validation and trimming
logic; keep the existing empty-name checks (e.g., name == "") in each caller
as-needed.
javiermtorres
approved these changes
Mar 3, 2026
| case context.Noop: | ||
| msg = fmt.Sprintf("No changes — specified volumes not present on server '%s': %v", serverName, sorted) | ||
| default: | ||
| msg = fmt.Sprintf("✓ Volumes removed for server '%s' (operation: %s): %v", serverName, string(res), sorted) |
There was a problem hiding this comment.
Again, the output strings could be magic-ified (so tests can reuse them and they can be easily changed later on).
Implement `mcpd config volumes remove <server-name> -- --<volume-name>` to remove volume mappings from MCP server runtime configurations. Also improves the existing `set` command: noop-aware messaging, consistent validation naming, and clearer withVolumes doc comment. Closes #200
peteski22
force-pushed
the
feature/config-volumes-remove
branch
from
b0e37d9 to
70da343
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove.go`:
- Around line 108-111: Currently the code clones server.RawVolumes into working
and falls back to an empty context.VolumeExecutionContext{}, which ignores
server.Volumes when RawVolumes is nil; change the initialization so you first
attempt maps.Clone(server.RawVolumes), if that result is nil then attempt
maps.Clone(server.Volumes), and only if that is still nil set working =
context.VolumeExecutionContext{}; update the code around the working variable
assignment (the clone of server.RawVolumes) so it checks server.Volumes as the
secondary source.
- Around line 146-153: The arg-parsing loop in remove.go currently accepts flags
like "--workspace=/tmp" as a literal volume name; update the loop that iterates
over args (the for _, arg := range args block) to reject any arg that contains
an '=' after the "--" prefix (e.g., detect strings.Contains(arg[len("--"):],
"=") or check for '=' in the derived name variable) and return an error such as
"invalid volume name '%s': flag assignments are not allowed for remove" instead
of treating them as names; keep the existing empty-name check on name
(strings.TrimSpace(strings.TrimPrefix(arg, "--"))), but add the '=' check before
trimming/using name so remove will not accept "--name=path".
In `@cmd/config/volumes/set.go`:
- Around line 113-116: The current logic initializes working from
server.RawVolumes and falls back to an empty context if nil, which can drop
existing mappings in server.Volumes; change the initialization so working is
maps.Clone(server.RawVolumes) if non-nil, otherwise maps.Clone(server.Volumes)
(and if both are nil use an empty context like
context.VolumeExecutionContext{}), updating the working variable creation in
set.go to prefer server.Volumes as the fallback before applying updates.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
cmd/config/volumes/cmd.gocmd/config/volumes/remove.gocmd/config/volumes/remove_test.gocmd/config/volumes/set.gocmd/config/volumes/set_test.go
Comment on lines
+108
to
+111
| working := maps.Clone(server.RawVolumes) | ||
| if working == nil { | ||
| working = context.VolumeExecutionContext{} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a fallback to server.Volumes when RawVolumes is nil.
At Line 108, removal starts from server.RawVolumes only. If RawVolumes is unset but Volumes is populated, the command behaves as if no volumes exist.
Suggested change
- working := maps.Clone(server.RawVolumes)
- if working == nil {
- working = context.VolumeExecutionContext{}
- }
+ working := maps.Clone(server.RawVolumes)
+ if working == nil {
+ working = maps.Clone(server.Volumes)
+ }
+ if working == nil {
+ working = context.VolumeExecutionContext{}
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| working := maps.Clone(server.RawVolumes) | |
| if working == nil { | |
| working = context.VolumeExecutionContext{} | |
| } | |
| working := maps.Clone(server.RawVolumes) | |
| if working == nil { | |
| working = maps.Clone(server.Volumes) | |
| } | |
| if working == nil { | |
| working = context.VolumeExecutionContext{} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/volumes/remove.go` around lines 108 - 111, Currently the code
clones server.RawVolumes into working and falls back to an empty
context.VolumeExecutionContext{}, which ignores server.Volumes when RawVolumes
is nil; change the initialization so you first attempt
maps.Clone(server.RawVolumes), if that result is nil then attempt
maps.Clone(server.Volumes), and only if that is still nil set working =
context.VolumeExecutionContext{}; update the code around the working variable
assignment (the clone of server.RawVolumes) so it checks server.Volumes as the
secondary source.
Comment on lines
113
to
116
| working := maps.Clone(server.RawVolumes) | ||
| if working == nil { | ||
| working = maps.Clone(server.Volumes) | ||
| } | ||
| if working == nil { | ||
| working = context.VolumeExecutionContext{} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use server.Volumes as fallback when RawVolumes is nil before applying updates.
At Line 113, updates begin from server.RawVolumes only. If that map is nil while server.Volumes contains mappings, existing volumes can be unintentionally discarded during set operations.
Suggested change
working := maps.Clone(server.RawVolumes)
if working == nil {
- working = context.VolumeExecutionContext{}
+ working = maps.Clone(server.Volumes)
+ }
+ if working == nil {
+ working = context.VolumeExecutionContext{}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| working := maps.Clone(server.RawVolumes) | |
| if working == nil { | |
| working = maps.Clone(server.Volumes) | |
| } | |
| if working == nil { | |
| working = context.VolumeExecutionContext{} | |
| } | |
| working := maps.Clone(server.RawVolumes) | |
| if working == nil { | |
| working = maps.Clone(server.Volumes) | |
| } | |
| if working == nil { | |
| working = context.VolumeExecutionContext{} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/volumes/set.go` around lines 113 - 116, The current logic
initializes working from server.RawVolumes and falls back to an empty context if
nil, which can drop existing mappings in server.Volumes; change the
initialization so working is maps.Clone(server.RawVolumes) if non-nil, otherwise
maps.Clone(server.Volumes) (and if both are nil use an empty context like
context.VolumeExecutionContext{}), updating the working variable creation in
set.go to prefer server.Volumes as the fallback before applying updates.
The remove command only accepts volume names (--workspace), not key=value pairs (--workspace=/tmp). Without this check, a mistyped --name=path arg would be parsed as a literal volume name that never matches, resulting in a silent noop.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/config/volumes/remove.go (1)
108-111:⚠️ Potential issue | 🟠 MajorUse
server.Volumesas fallback before mutating volume state.At Line 108,
workingis cloned fromserver.RawVolumesonly. IfRawVolumesis nil butVolumesis populated, this path can apply removals against an empty map and then persist that empty state viawithVolumes, causing unintended volume loss.#!/bin/bash # Verify whether RawVolumes is always initialised during load, and where it is assigned. # Expected result: # - If loader guarantees RawVolumes <- Volumes (or raw source), current code may be safe. # - If not, remove.go should fallback to server.Volumes before empty-map initialisation. rg -n --type=go -C4 '\bRawVolumes\b' rg -n --type=go -C6 'func .*Load\(' internal/context rg -n --type=go -C4 '\bwithVolumes\s*\(' cmd/config/volumesSuggested patch
- working := maps.Clone(server.RawVolumes) - if working == nil { - working = context.VolumeExecutionContext{} - } + working := maps.Clone(server.RawVolumes) + if working == nil { + working = maps.Clone(server.Volumes) + } + if working == nil { + working = context.VolumeExecutionContext{} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/volumes/remove.go` around lines 108 - 111, The code clones only server.RawVolumes into working which can be nil and cause removals to be applied to an empty map; change the logic in remove.go to choose a source map by preferring server.RawVolumes but falling back to server.Volumes before calling maps.Clone (then if both nil, initialize working = context.VolumeExecutionContext{}), and continue to pass working into withVolumes so you don't persist an unintended empty map; update references around maps.Clone(server.RawVolumes), the working variable, and the call to withVolumes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/volumes/remove_test.go`:
- Around line 41-118: Add a new test case in cmd/config/volumes/remove_test.go
within the existing tests slice that covers the regression where RawVolumes is
nil but Volumes is populated: create a case (e.g., name "rawVolumes nil but
volumes present") with serverName "filesystem", volumeArgs like
[]string{"--workspace"}, existingServers containing
context.ServerExecutionContext where Volumes has "workspace": "/Users/foo/repos"
and RawVolumes is nil (omit or set to nil), then assert expectedOutput and
expectedVolumes match the removal behavior (operation updated/deleted as
appropriate) to validate remove logic handles persisted runtime shapes;
reference the test harness that reads existingServers and uses
context.ServerExecutionContext, Volumes and RawVolumes fields to locate the
change.
---
Duplicate comments:
In `@cmd/config/volumes/remove.go`:
- Around line 108-111: The code clones only server.RawVolumes into working which
can be nil and cause removals to be applied to an empty map; change the logic in
remove.go to choose a source map by preferring server.RawVolumes but falling
back to server.Volumes before calling maps.Clone (then if both nil, initialize
working = context.VolumeExecutionContext{}), and continue to pass working into
withVolumes so you don't persist an unintended empty map; update references
around maps.Clone(server.RawVolumes), the working variable, and the call to
withVolumes accordingly.
Comment on lines
+41
to
+118
| { | ||
| name: "remove single volume", | ||
| serverName: "filesystem", | ||
| volumeArgs: []string{"--workspace"}, | ||
| existingServers: map[string]context.ServerExecutionContext{ | ||
| "filesystem": { | ||
| Name: "filesystem", | ||
| Volumes: context.VolumeExecutionContext{ | ||
| "workspace": "/Users/foo/repos", | ||
| "gdrive": "/mcp/gdrive", | ||
| }, | ||
| RawVolumes: context.VolumeExecutionContext{ | ||
| "workspace": "/Users/foo/repos", | ||
| "gdrive": "/mcp/gdrive", | ||
| }, | ||
| }, | ||
| }, | ||
| expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: updated): [workspace]", | ||
| expectedVolumes: context.VolumeExecutionContext{"gdrive": "/mcp/gdrive"}, | ||
| }, | ||
| { | ||
| name: "remove multiple volumes", | ||
| serverName: "filesystem", | ||
| volumeArgs: []string{"--workspace", "--gdrive"}, | ||
| existingServers: map[string]context.ServerExecutionContext{ | ||
| "filesystem": { | ||
| Name: "filesystem", | ||
| Volumes: context.VolumeExecutionContext{ | ||
| "workspace": "/Users/foo/repos", | ||
| "gdrive": "/mcp/gdrive", | ||
| "data": "vol", | ||
| }, | ||
| RawVolumes: context.VolumeExecutionContext{ | ||
| "workspace": "/Users/foo/repos", | ||
| "gdrive": "/mcp/gdrive", | ||
| "data": "vol", | ||
| }, | ||
| }, | ||
| }, | ||
| expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: updated): [gdrive workspace]", | ||
| expectedVolumes: context.VolumeExecutionContext{"data": "vol"}, | ||
| }, | ||
| { | ||
| name: "remove all volumes", | ||
| serverName: "filesystem", | ||
| volumeArgs: []string{"--workspace"}, | ||
| existingServers: map[string]context.ServerExecutionContext{ | ||
| "filesystem": { | ||
| Name: "filesystem", | ||
| Volumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, | ||
| RawVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, | ||
| }, | ||
| }, | ||
| expectedOutput: "✓ Volumes removed for server 'filesystem' (operation: deleted): [workspace]", | ||
| expectedVolumes: context.VolumeExecutionContext{}, | ||
| }, | ||
| { | ||
| name: "remove nonexistent volume is a noop", | ||
| serverName: "filesystem", | ||
| volumeArgs: []string{"--nonexistent"}, | ||
| existingServers: map[string]context.ServerExecutionContext{ | ||
| "filesystem": { | ||
| Name: "filesystem", | ||
| Volumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, | ||
| RawVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, | ||
| }, | ||
| }, | ||
| expectedOutput: "No changes — specified volumes not present on server 'filesystem': [nonexistent]", | ||
| expectedVolumes: context.VolumeExecutionContext{"workspace": "/Users/foo/repos"}, | ||
| }, | ||
| { | ||
| name: "server not found", | ||
| serverName: "nonexistent", | ||
| volumeArgs: []string{"--workspace"}, | ||
| existingServers: map[string]context.ServerExecutionContext{}, | ||
| expectedError: "server 'nonexistent' not found in configuration", | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression case for RawVolumes == nil with populated Volumes.
Current run cases always set both maps. Please add one case where only Volumes is populated, so remove behaviour is validated against persisted runtime data shapes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/volumes/remove_test.go` around lines 41 - 118, Add a new test case
in cmd/config/volumes/remove_test.go within the existing tests slice that covers
the regression where RawVolumes is nil but Volumes is populated: create a case
(e.g., name "rawVolumes nil but volumes present") with serverName "filesystem",
volumeArgs like []string{"--workspace"}, existingServers containing
context.ServerExecutionContext where Volumes has "workspace": "/Users/foo/repos"
and RawVolumes is nil (omit or set to nil), then assert expectedOutput and
expectedVolumes match the removal behavior (operation updated/deleted as
appropriate) to validate remove logic handles persisted runtime shapes;
reference the test harness that reads existingServers and uses
context.ServerExecutionContext, Volumes and RawVolumes fields to locate the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcpd config volumes remove <server-name> -- --<volume-name> [--<volume-name>...]command to remove volume mappings from MCP server runtime configurationssetcommand: noop-aware messaging, consistent validation naming (validateSetArgsCore), clearerwithVolumesdoc commentwithVolumeshelper shared by both commandsTest plan
go test ./cmd/config/volumes/... -count=1passesgo test ./... -count=1passes (full suite)deletedresult)--separator, invalid volume format, empty volume nameCloses #200
Summary by CodeRabbit
New Features
Improvements
Tests